Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

support Spectrum1D reading from file with flux in counts #1018

Merged
merged 3 commits into from
Feb 23, 2023
Merged

support Spectrum1D reading from file with flux in counts #1018

merged 3 commits into from
Feb 23, 2023

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Feb 21, 2023

this PR implements an exception to support reading a Spectrum1D from a file with fluxes in counts. This does not however address more general reading round-tripping such as the one reported in #997. Note also that this is just hardcoding an additional exception, Spectrum1D currently doesn't seem to do any unit checking on the inputs for flux, and so other units might result in the same error (including non-sensical units, like fluxes with units of meters).

Example (that used to raise an OSError: Could not identify column containing the flux):

import specutils
import numpy as np
import astropy.units as u

sp = specutils.Spectrum1D(flux=np.linspace(0,1,11)*u.Unit('count'), spectral_axis=np.random.random(11)*u.Unit('J'))
sp.write("spec.fits", overwrite=True)

specutils.Spectrum1D.read("spec.fits")

@codecov
Copy link

codecov bot commented Feb 21, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.13%. Comparing base (d4b3d2f) to head (fec6908).
Report is 134 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1018      +/-   ##
==========================================
+ Coverage   70.03%   70.13%   +0.09%     
==========================================
  Files          64       64              
  Lines        4339     4346       +7     
==========================================
+ Hits         3039     3048       +9     
+ Misses       1300     1298       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pllim
Copy link
Member

pllim commented Feb 21, 2023

u.Unit('J')

You sure you didn't mean Jy?

@pllim pllim requested review from dhomeier and rosteen February 21, 2023 21:17
@pllim
Copy link
Member

pllim commented Feb 21, 2023

Are we able to turn that example you provided into a test?

@kecnry
Copy link
Member Author

kecnry commented Feb 21, 2023 •
edited
Loading

You sure you didn't mean Jy?

No, this is for spectral axis, not flux, so I just chose a random compatible unit for the spectral axis. Swap out with Hz or um if you'd rather!

Are we able to turn that example you provided into a test?

Sure, we could provide a regression test for this same hardcoded case, but it really would only test that someone doesn't revert this commit... so I'm wondering if it makes more sense to write general tests with the longer-term solution? Happy to basically copy-paste this in to a test though just to make sure it doesn't raise errors.

@pllim
Copy link
Member

pllim commented Feb 21, 2023

No, this is for spectral axis, not flux

Doh! Ignore me... 😆

but it really would only test that someone doesn't revert this commit

Right, we wouldn't want regression, which could happen if we ever refactor the internals.

pllim
pllim reviewed Feb 22, 2023
View reviewed changes
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test could be written using parametrize and there is no need for random number generator. Please see suggestions.

Not sure what's up with the change log, maybe @rosteen only updates it on release? https://github.com/astropy/specutils/blob/main/CHANGES.rst

specutils/tests/test_spectrum1d.py Outdated Show resolved Hide resolved
specutils/tests/test_spectrum1d.py Outdated Show resolved Hide resolved
specutils/tests/test_spectrum1d.py Outdated Show resolved Hide resolved
specutils/tests/test_spectrum1d.py Outdated Show resolved Hide resolved
@rosteen
Copy link
Contributor

rosteen commented Feb 22, 2023

Not sure what's up with the change log, maybe @rosteen only updates it on release? https://github.com/astropy/specutils/blob/main/CHANGES.rst

Yeah, I've been updating it when I do a release. I'm certainly not opposed to people updating it when they make a PR though!

I'm going to approve this, but generally agree with your suggestions @pllim . I'm about to log off for vacation but feel free to merge this once those are addressed.

dhomeier
dhomeier requested changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@dhomeier dhomeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the requested changes, otherwise this looks sensible. Yes, Spectrum1D.__init__ does not make any checks on flux PhysicalType, but there you are directly passing your quantity. parsing_utils are searching for some compatible data fields in an input file, so it makes sense to be more conservative.

@kecnry @pllim
Apply suggestions from code review
fec6908 
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
@kecnry kecnry force-pushed the spectrum1d-read-flux-in-counts branch from 1ab55d0 to fec6908 Compare February 23, 2023 14:22
@pllim
Copy link
Member

pllim commented Feb 23, 2023

@dhomeier , does the PR look okay to you now, or is there something else? Thanks!

@dhomeier
Copy link
Contributor

Everything you had requested as far as I can see; good to me too, thanks!

@pllim pllim merged commit 75a6cba into astropy:main Feb 23, 2023
@pllim
Copy link
Member

pllim commented Feb 23, 2023

Thanks, all!

@kecnry kecnry deleted the spectrum1d-read-flux-in-counts branch February 23, 2023 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pllim pllim pllim left review comments

@rosteen rosteen rosteen approved these changes

@dhomeier dhomeier dhomeier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kecnry @pllim @rosteen @dhomeier